Queue and batch dependency actions #181

superhawk610 · 2018-08-31T07:36:38Z

Related Issue: #33
Related PR: #103

Summary:
Currently, installing/updating/deleting a dependency locks that project and blocks further actions until it has completed. This is a huge hindrance to common tasks like installing multiple dependencies, but due to the way package managers work it's generally a bad idea (read: impossible) to perform multiple yarn adds or the like in parallel.

This PR changes the way dependencies are installed/uninstalled - projects are never locked within the UI, instead creating queued actions when the user attempts to change a dependency while another dependency action is aleady in progress. Since multiple dependencies can be installed/uninstalled with a single command (yarn add foo bar baz), we can batch together queued actions while we wait for the active commands to complete.

The major breaking changes in this PR include:

queue.reducer.js: This reducer takes the place of package-json-locked.reducer.js. When an action would change a dependency while another is already in progress, it is instead pushed into that project's queue. (NOTE: if you still need to check if that project's package.json is locked, this reducer exports a selector getPackageJsonLockedForProjectId that functions exactly as it did previously.)
Dependency Action Restructure: All dependency-related actions have been restructured (and a few new have been added) as follows:

// indicate that the user would like to change a dependency
export const ADD_DEPENDENCY = 'ADD_DEPENDENCY';
export const UPDATE_DEPENDENCY = 'UPDATE_DEPENDENCY';
export const DELETE_DEPENDENCY = 'DELETE_DEPENDENCY';

// spawn a child process to actually change the dependencies
export const INSTALL_DEPENDENCIES_START = 'INSTALL_DEPENDENCIES_START';
export const INSTALL_DEPENDENCIES_ERROR = 'INSTALL_DEPENDENCIES_ERROR';
export const INSTALL_DEPENDENCIES_FINISH = 'INSTALL_DEPENDENCIES_FINISH';
export const UNINSTALL_DEPENDENCIES_START = 'UNINSTALL_DEPENDENCIES_START';
export const UNINSTALL_DEPENDENCIES_ERROR = 'UNINSTALL_DEPENDENCIES_ERROR';
export const UNINSTALL_DEPENDENCIES_FINISH = 'UNINSTALL_DEPENDENCIES_FINISH';

// add a dependency to a project's queue
export const QUEUE_DEPENDENCY_INSTALL = 'QUEUE_DEPENDENCY_INSTALL';
export const QUEUE_DEPENDENCY_UNINSTALL = 'QUEUE_DEPENDENCY_UNINSTALL';

// start working on the next action in the project's queue
export const START_NEXT_ACTION_IN_QUEUE = 'START_NEXT_ACTION_IN_QUEUE';

Non-blocking UI: Buttons that trigger dependency changes are now always enabled, and don't need to check whether the current project's package.json is locked since that logic is handled by dependency.saga.js.
Testing: All relevant components with testing have been updated to pass with these new changes.

superhawk610 · 2018-08-31T07:38:45Z

While all tests are passing, I am not actually able to test functionality on Windows until a few breaking bugs are fixed (#176).

Flow typing is also broken - I probably need to add a separate Dependency type used specifically during installs/uninstalls with all optional properties, as most of the errors stem from dependencies missing required properties.

superhawk610 · 2018-08-31T15:12:39Z

While I'm thinking about it, I need to update the dependency pane UI to reflect the new queued- statuses. Probably also worth changing button next from Add to Project to Queue ?

joshwcomeau · 2018-08-31T18:47:09Z

@superhawk610 The breaking bugs should be fixed, hope it's testable now!

While I'm thinking about it, I need to update the dependency pane UI to reflect the new queued- statuses. Probably also worth changing button next from Add to Project to Queue ?

Hm, I think "queue" would be confusing for users, since they probably aren't thinking about it in terms of queueing an install. I think it can stay as "Add to Project", or maybe "Install", since install implies it'll take some time?

src/components/AddDependencySearchResult/AddDependencySearchResult.js

melanieseltzer · 2018-08-31T22:39:20Z

Totally ignore this if you're already aware (cause not sure what you're still working on since it's WIP), but I'm getting some errors when testing the functionality when clicking 'Add to Project' and also 'Delete'. The project seems to instantly get added to the dependency list, that seems... not right. And Delete fails entirely with the same sort of error message.

uncaught at addDependency TypeError: Cannot read property '#<Object>' of undefined

uncaught at deleteDependency TypeError: Cannot read property '#<Object>' of undefined

superhawk610 · 2018-08-31T23:22:25Z

@melanieseltzer yeah, UI integration was the WIP part 😁

If it makes you feel any better, I just flat-out don't have a dependencies pane in my current revision, so it's safe to say there's a bit left to do there. Thanks for the gif, that gives me a good place to start.

superhawk610 · 2018-09-01T01:44:46Z

Alright, the UI is up to date with the rest of the PR and it should be entirely functional.

The only visual bug, the Delete button in the dependency management pane, is detailed in #187.

melanieseltzer · 2018-09-01T05:30:50Z

@superhawk610 Sick 👌 I haven't had a chance to look at the code, but one thing I noticed with the UI is that if I queue up three or more installs, once the first finishes installing and the spinner stops, the remaining will immediately all at once switch to 'Installing' instead of maintaining the queue.

melanieseltzer · 2018-09-01T05:33:05Z

And some stuff related to #187 I think...

superhawk610 · 2018-09-01T07:25:25Z

once the first finishes installing and the spinner stops, the remaining will immediately all at once switch to 'Installing' instead of maintaining the queue.

That's intended 😊 Instead of installing all dependencies serially (yarn add foo, yarn add bar), they are batched and installed in parallel (yarn add foo bar).

joshwcomeau · 2018-09-01T12:05:26Z

Omg this is great 🎉🎉🎉🎉🎉🎉🎉Haven't looked at the code yet, just tried it locally, and it's such a better experience omg.

One UX thought: I wonder if we need to expose the idea of a queue at all to the user... Right now there's a clear difference between a dependency queued to install, and one installing. As you're all too familiar, there's a good reason for this difference, but I'm not convinced the user has to know. Maybe we should treat all queued and installing dependencies the same, saying that they're all "installing"?

The one weird thing about that is that they'll finish at different times (the very first dependency will finish, and then all the rest in a batch), but I think that's ok!

But yeah, no change necessary. I'm not convinced my suggestion is good (I'd love to hear from @melanieseltzer and others!), and even if it is, it's easily something that can be addressed later in another PR.

I also noticed the issues that @melanieseltzer brought up; there's some weird double-button thing happening when it's queued, and a similar issue when it's deleting.

joshwcomeau

Phew! Made it through.

I didn't review the tests very thoroughly, but otherwise I gave it a good pass, ran everything locally. Overall it looks great!

I did have some questions and some suggested changes, though. Let me know if any of my suggestions aren't practical.

The only bugs I found were the ones Melanie caught. I think they can be dealt with in a subsequent PR though, shouldn't be blocking.

Excited to see this land, it's such a better experience!

src/components/AddDependencySearchResult/AddDependencySearchResult.js

src/components/DeleteDependencyButton/DeleteDependencyButton.js

src/components/DependencyManagementPane/DependencyManagementPane.js

src/services/dependencies.service.js

src/services/read-from-disk.service.js

src/store/index.js

joshwcomeau

Yayyy this looks great! Thanks so much for making all the requested changes :)

joshwcomeau · 2018-09-02T15:06:00Z

src/reducers/queue.reducer.js

+};
+
+type State = {
+  [projectId: string]: Array<QueueEntry>,


Yay, you caught that my suggestion was missing the Array<> haha

- update dependencies reducer and tests - add queue reducer and tests - move snapshots to separate directory - add `queued-` statuses

- update depedency saga tests - add loadProjectDependencies to read-from-disk.service

- update tests to reflect structural changes - exclude `queue` state from redux-storage

superhawk610 requested review from joshwcomeau and melanieseltzer August 31, 2018 07:36

superhawk610 mentioned this pull request Aug 31, 2018

Queue/group actions that lock package.json #103

Closed

joshwcomeau mentioned this pull request Aug 31, 2018

Stop scanning home directory for projects #176

Merged

melanieseltzer reviewed Aug 31, 2018

View reviewed changes

src/components/AddDependencySearchResult/AddDependencySearchResult.js Outdated Show resolved Hide resolved

superhawk610 force-pushed the queue-dependencies branch from 0f4a7ed to 01b4496 Compare September 1, 2018 01:37

superhawk610 changed the title ~~[WIP] Queue and batch dependency actions~~ Queue and batch dependency actions Sep 1, 2018

joshwcomeau suggested changes Sep 1, 2018

View reviewed changes

AWolf81 mentioned this pull request Sep 2, 2018

Project configuration #120

Merged

31 tasks

joshwcomeau approved these changes Sep 2, 2018

View reviewed changes

superhawk610 added 7 commits September 2, 2018 11:14

update redux action structure

504de22

- update dependencies reducer and tests - add queue reducer and tests - move snapshots to separate directory - add `queued-` statuses

update dependency saga for queued actions

e7c4d7e

- update depedency saga tests - add loadProjectDependencies to read-from-disk.service

update UI to use new queue methods

29d7d98

fix flow typing

2b90e45

update UI to reflect new queue/batch actions

791011a

- update tests to reflect structural changes - exclude `queue` state from redux-storage

add queue indicator to dependency list

7deb8bd

resolve review comments

799beb5

joshwcomeau force-pushed the queue-dependencies branch from 6afbcc3 to 799beb5 Compare September 2, 2018 15:21

joshwcomeau merged commit 280421e into master Sep 2, 2018

joshwcomeau deleted the queue-dependencies branch September 2, 2018 15:22

joshwcomeau mentioned this pull request Sep 2, 2018

Clear state menu item #200

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Queue and batch dependency actions #181

Queue and batch dependency actions #181

superhawk610 commented Aug 31, 2018 •

edited

Loading

superhawk610 commented Aug 31, 2018 •

edited

Loading

superhawk610 commented Aug 31, 2018

joshwcomeau commented Aug 31, 2018

melanieseltzer commented Aug 31, 2018

superhawk610 commented Aug 31, 2018

superhawk610 commented Sep 1, 2018

melanieseltzer commented Sep 1, 2018

melanieseltzer commented Sep 1, 2018

superhawk610 commented Sep 1, 2018 •

edited

Loading

joshwcomeau commented Sep 1, 2018

joshwcomeau left a comment

joshwcomeau left a comment

joshwcomeau Sep 2, 2018

Queue and batch dependency actions #181

Queue and batch dependency actions #181

Conversation

superhawk610 commented Aug 31, 2018 • edited Loading

superhawk610 commented Aug 31, 2018 • edited Loading

superhawk610 commented Aug 31, 2018

joshwcomeau commented Aug 31, 2018

melanieseltzer commented Aug 31, 2018

superhawk610 commented Aug 31, 2018

superhawk610 commented Sep 1, 2018

melanieseltzer commented Sep 1, 2018

melanieseltzer commented Sep 1, 2018

superhawk610 commented Sep 1, 2018 • edited Loading

joshwcomeau commented Sep 1, 2018

joshwcomeau left a comment

Choose a reason for hiding this comment

joshwcomeau left a comment

Choose a reason for hiding this comment

joshwcomeau Sep 2, 2018

Choose a reason for hiding this comment

superhawk610 commented Aug 31, 2018 •

edited

Loading

superhawk610 commented Aug 31, 2018 •

edited

Loading

superhawk610 commented Sep 1, 2018 •

edited

Loading